chore: use derive for eq checks #3521
Conversation
📝 WalkthroughWalkthroughThis change introduces automated equality checking for billing domain structs using code generation. A new generator directive and derived equality functions replace manual reflection-based comparisons across multiple files, while a new annotation equality method is added to the models package. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
turip
force-pushed
the
chore/use-derive-for-eq-checks
branch
from
dd2bbf7 to
e1bf64f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (6)
openmeter/billing/derived.gen.go(1 hunks)openmeter/billing/generate.go(1 hunks)openmeter/billing/invoicedetailedline.go(1 hunks)openmeter/billing/invoiceline.go(2 hunks)openmeter/billing/invoicelinediscount.go(5 hunks)pkg/models/annotation.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Files:
openmeter/billing/invoicedetailedline.goopenmeter/billing/derived.gen.goopenmeter/billing/invoiceline.goopenmeter/billing/invoicelinediscount.goopenmeter/billing/generate.gopkg/models/annotation.go
🧠 Learnings (2)
📚 Learning: 2025-04-21T08:32:31.689Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In `productcatalog.UsageBasedRateCard`, the `BillingCadence` field is a non-pointer `isodate.Period`, while in `productcatalog.FlatFeeRateCard`, `BillingCadence` is a pointer type (`*isodate.Period`). This means `MonthPeriod` should be used directly for `UsageBasedRateCard` (not `&MonthPeriod`).
Applied to files:
openmeter/billing/derived.gen.goopenmeter/billing/generate.go
📚 Learning: 2025-03-07T12:17:43.129Z
Learnt from: GAlexIHU
Repo: openmeterio/openmeter PR: 2383
File: openmeter/entitlement/metered/lateevents_test.go:37-45
Timestamp: 2025-03-07T12:17:43.129Z
Learning: In the OpenMeter codebase, test files like `openmeter/entitlement/metered/lateevents_test.go` may use variables like `meterSlug` and `namespace` without explicit declarations visible in the same file. This appears to be an accepted pattern in their test structure.
Applied to files:
openmeter/billing/invoiceline.go
🧬 Code graph analysis (2)
openmeter/billing/derived.gen.go (8)
openmeter/billing/invoicedetailedline.go (1)
DetailedLineBase(42-68)pkg/models/model.go (4)
ManagedResource(23-31)ManagedModelWithID(179-182)NamespacedModel(204-206)ManagedModel(119-125)openmeter/billing/totals.go (1)
Totals(9-26)openmeter/billing/invoiceline.go (4)
LineBase(96-123)Period(51-54)UsageBasedLine(632-645)SubscriptionReference(194-199)pkg/models/annotation.go (1)
Annotations(5-5)openmeter/billing/invoicelinediscount.go (5)
LineDiscountBase(20-25)AmountLineDiscount(58-66)AmountLineDiscountManaged(94-97)UsageLineDiscount(203-208)UsageLineDiscountManaged(240-243)openmeter/billing/discount.go (1)
DiscountReason(147-152)pkg/timeutil/closedperiod.go (1)
ClosedPeriod(8-11)
pkg/models/annotation.go (1)
api/api.gen.go (1)
Annotations(1049-1049)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
openmeter/billing/generate.go (1)
1-3: Nice! This sets up the code generation foundation.The
go:generatedirective will produce the equality helpers inderived.gen.go, which is exactly what this PR aims to achieve. Clean and straightforward.openmeter/billing/invoicedetailedline.go (1)
116-118: LGTM! Clean switch to generated equality.This now uses the generated helper which will correctly handle decimal comparisons using their
.Equal()methods instead of reflect.openmeter/billing/invoicelinediscount.go (1)
37-39: All the discount equality methods look good!Consistent migration to generated helpers across all discount types. The generated code will properly compare decimal amounts using their type-specific
.Equal()methods.Also applies to: 82-84, 109-111, 228-230, 255-257
openmeter/billing/invoiceline.go (1)
125-127: Both equality methods migrated successfully!
LineBase.EqualandUsageBasedLine.Equalnow use their generated helpers. Note thatUsageBasedLine.Equalcorrectly passesotherdirectly since it's already a pointer parameter. The generated helpers will handle all the decimal fields properly.Also applies to: 647-649
openmeter/billing/derived.gen.go (1)
10-186: The generated equality helpers look solid!The code correctly:
- Uses
.Equal()methods for all decimal types (solving the core issue from the PR description)- Handles nil checks consistently for all pointer types
- Uses
.Equal()for time.Time comparisons- Delegates to existing
.Equal()methods on nested typesOne thing to note: Line 36 calls
this.Annotations.Equal(that.Annotations), which relies on the reflect-based method added inpkg/models/annotation.go. If annotations can contain decimal values, that could still have comparison issues (already flagged in that file's review).
| func (a Annotations) Equal(other Annotations) bool { | ||
| if a == nil || other == nil { | ||
| return a == nil && other == nil | ||
| } | ||
|
|
||
| if len(a) != len(other) { | ||
| return false | ||
| } | ||
|
|
||
| for k, v := range a { | ||
| otherV, ok := other[k] | ||
| if !ok { | ||
| return false | ||
| } | ||
|
|
||
| if !reflect.DeepEqual(v, otherV) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Consider the implications of using reflect.DeepEqual here.
The PR description specifically mentions moving away from reflect.DeepEqual because it doesn't correctly handle decimal types with different exponent representations (e.g., 8*2^2 vs 32*2^0). If Annotations can contain decimal values, this implementation will have the same problem.
Since Annotations is map[string]interface{}, have you considered:
- Whether decimal values are commonly stored in annotations?
- If so, whether a type switch could be used to call
.Equal()on decimal types specifically? - Whether goderive could generate equality for this map type?
</comment_end>
🤖 Prompt for AI Agents
In pkg/models/annotation.go around lines 93 to 113, the current use of
reflect.DeepEqual will miscompare decimal values that have different internal
representations; change the comparison loop to detect known decimal types (e.g.,
shopspring/decimal.Decimal or your project's decimal type) via a type switch and
call their Equal (or Cmp==0) method, falling back to reflect.DeepEqual only for
non-decimal types; ensure you also handle pointer vs non-pointer decimal
variants and return false if types differ but are comparable; this preserves
correct equality for decimals while keeping the existing behavior for other
annotation value types.
Overview
This patch adds https://github.com/awalterschulze/goderive for generating equality checks. I would want to have this approach instead of manually writing the equality checks as they are less error prone.
In theory it can also be used to generate the clone methods, however there's a bug with time handling, so that is not something I would do right now: awalterschulze/goderive#91.
Right now we cannot use reflect.deepequals for equality checks as for example alpacadecimal when in fallback mode (e.g. uses the shopspring decimal) uses an exponential representation of the number, for example
8*2^2(==32). However the library does not normalize the exponent, so it can be also represented as32*2^0. These values are not deepequal, however the type's Equal member returns the good value.Summary by CodeRabbit